ensure moderated file transfers only perform allowed operations#39091
Conversation
capnspacehook
added
server-access
sftp
|
improve security is a bit vague - can we come up with a better changelog entry? |
There was a problem hiding this comment.
When do we plan to address this TODO?
There was a problem hiding this comment.
Soon, it's tracked here: https://github.com/gravitational/teleport-private/issues/1401. It wasn't included in this fix because this fix will be added to a patch release, and addressing this TODO is technically breaking behavior so best saved for another PR and not backported.
|
@zmb3 how's the updated changelog entry? |
capnspacehook
requested review from
Tener and
avatus
and removed request for
GavinFrazar and
hugoShaka
|
@zmb3 can I get another review please? |
zmb3
left a comment
zmb3
left a comment
There was a problem hiding this comment.
We're almost there.
I think we can remove the custom buffered reader type, and I think the changelog entry should mention that only 1 in-flight request is allowed now.
Otherwise looks good!
|
@zmb3 the changelog entry look like what you had in mind? |
capnspacehook
force-pushed
the
capnspacehook/file-transfer-req-server-check
branch
from
a942cce to
f60120e
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
@capnspacehook See the table below for backport results.
|
4 participants
Fixes https://github.com/gravitational/teleport-private/issues/1055.
Closes https://github.com/gravitational/teleport-private/issues/1391.
To reduce complexity and the risk of file transfer requests getting reused again (see the above issue 1055) pending file transfer requests are now limited to one per session. Creating multiple in-flight file transfer requests for moderated sessions didn't properly work before anyway, so I don't think we're losing anything by doing this. The SFTP server now also only allows necessary operations for the session's approved file transfer request if there is one present.
This change should be backwards compatible as the API for managing file transfer requests has not changed, only previously non-functional behavior has been removed. Since Nodes re-exec themselves to start an SFTP server, Node processes now sending file transfer request info to SFTP server processes shouldn't pose a compatibility issue.
Reviewed already in https://github.com/gravitational/teleport-private/pull/1399, just need approvals to get this merged.
changelog: only allow necessary operations during moderated file transfers and limit in-flight file transfer requests to one per session